Automated comment from CodeApprove ➜

⏳ Approval Pending (1 unresolved comments)
Approval will be granted automatically when all comments are resolved

In: dev-scripts/run-e2e-tests:

> Line 40
# If the first positional argument starts with 'http', strip it to export it as

Just bouncing this as a thought here: I’m a bit on the fence with providing two optional positional arguments, as that is ambiguous to parse. Without reading this code comment, it might not be 100% clear from the outside how this script disambiguates the two arguments.

In my mind, the ideal and most “conventional” way for the script API would be:

• Make E2E_BASE_URL a named flag, like --baseUrl http://...
• Separate the script flags from the Playwright flags via a -- separator

That way, you could do:

./run-e2e-tests --baseUrl http://localhost:123 -- --grep 'foo'

That being said, I think we’ve never used the -- separator in our scripts, so I don’t know how much work it would be, or whether it’d be worth to invest that for an internal dev script.

So depending on what your thoughts are, to me it would be fine to leave this as proposed, though I think we could then address the flag parsing behaviour more explicitly in the help output.

👀 @jdeanwallace it's your turn please take a look

Automated comment from CodeApprove ➜

In: dev-scripts/run-e2e-tests:
Oh yeah, that makes sense. I was intrigued by the -- idea, so did implement it here.

Can you check once again if all is in order?

👀 @jotaen4tinypilot it's your turn please take a look

Automated comment from CodeApprove ➜

In: dev-scripts/run-e2e-tests:
Neat!

In: dev-scripts/run-e2e-tests:

> Line 88
npx playwright test "${PLAYWRIGHT_ARGS[@]+"${PLAYWRIGHT_ARGS[@]}"}"

This is some wacky bash!

Can we add a comment explaining what it's doing? I feel like nobody will understand it except for like the top 5% of bash developers.

👀 @jdeanwallace, @jotaen4tinypilot it's your turn please take a look

Automated comment from CodeApprove ➜

⏳ Approval Pending (3 unresolved comments)
Approval will be granted automatically when all comments are resolved

In: dev-scripts/run-e2e-tests:
🎉

In: dev-scripts/run-e2e-tests:

> Line 19
  --                      Optional. Indicate the end of this script's CLI

I’m wondering whether we could consolidate -- and PLAYWRIGHT_ARGS into a single help item. My reasoning would be that they are not independent from each other, but it’s either both or none (i.e., you specify -- to enter Playwright-args land).

This should in theory be clear from the Usage string definition, which we only reference here, but I think there might still be potential room for misunderstanding.

In: dev-scripts/run-e2e-tests:

> Line 48
      shift

What would you think about initializing $PLAYWRIGHT_ARGS as empty array (PLAYWRIGHT_ARGS=()) above the while, and then doing the ("$@") assignment here? I’m thinking that this could make the code more cohesive, and thus slightly easier to follow – like:

--)
  # Stop parsing command-line arguments, and capture all remaining
  # args to pass them through to Playwright.
  shift
  PLAYWRIGHT_ARGS=("$@")
  break

In: dev-scripts/run-e2e-tests:

> Line 88
npx playwright test "${PLAYWRIGHT_ARGS[@]+"${PLAYWRIGHT_ARGS[@]}"}"

Would it be safe to always pass on $PLAYWRIGHT_ARGS to npx ..., even if it’s empty? Or is there a scenario where Playwright might complain?

👀 @jdeanwallace, @mtlynch it's your turn please take a look

Automated comment from CodeApprove ➜

In: dev-scripts/run-e2e-tests:
Done.

In: dev-scripts/run-e2e-tests:

> Line 49
      shift

Done.

In: dev-scripts/run-e2e-tests:

> Line 93
npx playwright test "${PLAYWRIGHT_ARGS[@]+"${PLAYWRIGHT_ARGS[@]}"}"

Can we add a comment explaining what it's doing?

Done.

Would it be safe to always pass on $PLAYWRIGHT_ARGS to npx ..., even if it’s empty? Or is there a scenario where Playwright might complain?

Yes, if $PLAYWRIGHT_ARGSis empty, nothing is passed to npx.

👀 @mtlynch it's your turn please take a look

Automated comment from CodeApprove ➜

Approved: I have approved this change on CodeApprove and all of my comments have been resolved.